Handle empty module on the course structure saving #7649

merkushin · 2024-07-31T05:57:42Z

Resolves #7625

The problem described in the issue happens when, while following the tour, the user doesn't set the module title.

The course structure can't be saved in this case, and it causes the issue that the expected “Edit Lesson” link doesn't appear in the toolbar.

Proposed Changes

Adjust the text in the tour to instruct the user to set a title for the module.
Set default titles for modules on saving.

Testing Instructions

On a new website, create a course, and follow the next steps before saving/publishing the course.
Take the tour and on the third step (“Adding a module”) make sure we mention the need to add the title for the module: “Then click the Module option and give it a name.”
Add a few modules without setting titles for them.
Save the draft.
Make sure each module without a name got a title like “Module N” where N is an ordinal number.

Pre-Merge Checklist

github-actions · 2024-07-31T06:01:25Z

Test the previous changes of this PR with WordPress Playground.

github-actions · 2024-07-31T06:03:34Z

WordPress Dependencies Report

The github-action-wordpress-dependencies-report action has detected some script changes between the commit 8eab758 and trunk. Please review and confirm the following are correct before merging.

Script Handle	Added Dependencies	Removed Dependencies	Total Size	Size Diff
`admin/tour/course-tour/index.js`			35.9 kB	+8 B ( +0.03% 🔼 )

This comment was automatically generated by the github-action-wordpress-dependencies-report action.

github-actions · 2024-07-31T06:40:41Z

Test the previous changes of this PR with WordPress Playground.

m1r0

Looks good and works as described. 👍

As a side note, I've noticed that on the Mac, the tour hides the module so it's hard to see. This could be addressed in another PR.

Imran92

LGTM, just added two small comments.

Imran92 · 2024-07-31T16:47:15Z

includes/class-sensei-course-structure.php

+				}
+
+				// Ignore items that are not modules or have a title already.
+				if ( 'module' !== $item['type'] || ! empty( $item['title'] ) ) {


Nitpick: Just an edge case as we are trying to work against user-made scenarios - can we trim the title and then check if it's empty? Because otherwise, it doesn't assign a name to the module if the title is just one or more spaces. Does it make sense?

Imran92 · 2024-07-31T17:08:34Z

includes/class-sensei-course-structure.php

@@ -230,7 +230,8 @@ private function get_modules(): array {
 	 * @return bool|WP_Error
 	 */
 	public function save( array $raw_structure ) {
-		$structure = $this->sanitize_structure( $raw_structure );
+		$raw_structure = $this->ensure_modules_have_names( $raw_structure );


Has this check become redundant now?

sensei/includes/class-sensei-course-structure.php

Lines 897 to 902 in 53dbffa

if ( 'module' === $raw_item['type'] ) {

return new WP_Error(

'sensei_course_structure_modules_missing_title',

__( 'Please ensure all modules have a name before saving.', 'sensei-lms' )

);

}

renatho · 2024-07-31T17:37:41Z

Hey! 👋
Just a suggestion. I thought we would implement it as a solution applied only to the tour (populating the module names if needed as part of one of the steps). Wouldn't it be better?

Imran92 · 2024-07-31T17:47:41Z

Thanks for raising it @renatho !

Hey! 👋
Just a suggestion. I thought we would implement it as a solution applied only to the tour (populating the module names if needed as part of one of the steps). Wouldn't it be better?

It was exactly my thought and I agree it would be better (That's why my suggestion here p1722358285119989/1722355438.888029-slack-C02NWDZBL0H was to add the names before "triggering" save, we trigger a save during the tour). I thought about commenting the same here, but later thought as this is already implemented, it'd make us re-do the whole thing, that's why I approved it, thinking about the cost vs. benefit. But I would've preferred doing it only for the tour to keep the flow of the tour smooth and errorless, and enforce user to explicitly give modules a Title as before and not saving otherwise in rest of the cases.

merkushin · 2024-07-31T21:35:57Z

@renatho @Imran92

I thought we would implement it as a solution applied only to the tour (populating the module names if needed as part of one of the steps). Wouldn't it be better?

That's why my suggestion here p1722358285119989/1722355438.888029-slack-C02NWDZBL0H was to add the names before "triggering" save, we trigger a save during the tour.

Yes, initially tried to do that on the frontend, but didn't find a proper way to update titles, and switched to a more straightforward solution.

Also, speaking about the frontend, I noticed that we only show the error message at the top of the screen, but don't even highlight the fields that require user's action (and I think because of placeholders it might not be obvious for some users). So I was thinking that setting the default title on the backend is a quick solution to this problem as well as it forgives some user mistakes, and we don't need to work on UX and design for that.

I'm going to look maybe for a quick dirty solution for the approach Imran suggested.

merkushin · 2024-08-01T02:26:28Z

@renatho @Imran92 here is the new PR with setting titles only in the tour: #7652

merkushin added this to the 4.24.2 milestone Jul 31, 2024

merkushin requested a review from a team July 31, 2024 05:57

merkushin self-assigned this Jul 31, 2024

merkushin added 4 commits July 31, 2024 00:33

Set the default name for modules with empty titles

464f517

Test setting default names for modules without titles

183171a

Add changelog entry

4738c7a

Don't enforce the return type for the closure

8eab758

merkushin force-pushed the fix/handle-empty-module-on-save branch from e80cc03 to 8eab758 Compare July 31, 2024 06:36

m1r0 approved these changes Jul 31, 2024

View reviewed changes

Imran92 approved these changes Jul 31, 2024

View reviewed changes

merkushin closed this Aug 1, 2024

donnapep deleted the fix/handle-empty-module-on-save branch August 1, 2024 15:57

donnapep removed this from the 4.24.2 milestone Aug 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle empty module on the course structure saving #7649

Handle empty module on the course structure saving #7649

merkushin commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

github-actions bot commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024

m1r0 left a comment

Imran92 left a comment

Imran92 Jul 31, 2024

Imran92 Jul 31, 2024

renatho commented Jul 31, 2024

Imran92 commented Jul 31, 2024 •

edited

Loading

merkushin commented Jul 31, 2024

merkushin commented Aug 1, 2024

	if ( 'module' === $raw_item['type'] ) {
	return new WP_Error(
	'sensei_course_structure_modules_missing_title',
	__( 'Please ensure all modules have a name before saving.', 'sensei-lms' )
	);
	}

Handle empty module on the course structure saving #7649

Handle empty module on the course structure saving #7649

Conversation

merkushin commented Jul 31, 2024

Proposed Changes

Testing Instructions

Pre-Merge Checklist

github-actions bot commented Jul 31, 2024

github-actions bot commented Jul 31, 2024 • edited Loading

WordPress Dependencies Report

github-actions bot commented Jul 31, 2024

m1r0 left a comment

Choose a reason for hiding this comment

Imran92 left a comment

Choose a reason for hiding this comment

Imran92 Jul 31, 2024

Choose a reason for hiding this comment

Imran92 Jul 31, 2024

Choose a reason for hiding this comment

renatho commented Jul 31, 2024

Imran92 commented Jul 31, 2024 • edited Loading

merkushin commented Jul 31, 2024

merkushin commented Aug 1, 2024

github-actions bot commented Jul 31, 2024 •

edited

Loading

Imran92 commented Jul 31, 2024 •

edited

Loading